Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add direct containers #788

Merged
merged 1 commit into from
May 7, 2015
Merged

Add direct containers #788

merged 1 commit into from
May 7, 2015

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented May 6, 2015

You can use the containers by calling:

class FooHistory < ActiveFedora::Base
   directly_contains :files, predicate:
   ::RDF::URI.new("http://example.com/hasFiles"), class_name: 'Thing'
end

if you don't specify a class_name it will default to ActiveFedora::File

@jcoyne jcoyne self-assigned this May 6, 2015
@jcoyne jcoyne added this to the May Hydra PCDM milestone May 6, 2015
@awead
Copy link
Contributor

awead commented May 6, 2015

Since @hectorcorrea and @terrellt are already looking at this, I'm giving this a tentative 👍 as long as it can create things that look like what's been outlined in https://github.com/awead/sufia-pcdm. I'm happy to give this a try, if you want.

tpendragon
tpendragon reviewed May 6, 2015
View reviewed changes
spec/integration/direct_container_spec.rb
end
end

describe "#to_a" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a test for what happens when there are no files?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. See line 18

@hectorcorrea
Copy link
Member

@jcoyne This implementation only address the scenario in which the direct container is created with a hasMemberRelation.

In other words, it does not let you create direct containers with isMemberOfRelation instead of hasMemberRelation, right?

I am asking because we might need support for isMemberRelation in the implementation of PCDM (see here: https://wiki.duraspace.org/display/FEDORA4x/LDP-PCDM-F4+In+Action#LDP-PCDM-F4InAction-Ordering-CreateDirectContainer)

@jcoyne
Copy link
Member Author

jcoyne commented May 6, 2015

@hectorcorrea I'm not seeing where isMemberRelation is used in PCDM (in the link you sent me). Can you help me find that?

@hectorcorrea
Copy link
Member

@jcoyne Look for

Like the "pages/" DirectContainer in an earlier example, the "orderProxies/" includes the ldp:membershipResource property ("raven/"). However, it is important to point out that the "orderProxies/" DirectContainer *does not* have the ldp:hasMemberRelation property defined, but instead uses ldp:isMemberOfRelation of "ore:proxyIn".
By using ldp:isMemberOfRelation, the auto-created triple resulting from the addition of a new child resource within "orderProxies/" will take the form

<new-resource> <ore:proxyIn> <http://localhost:8080/fcrepo/rest/objects/raven/>

@hectorcorrea
Copy link
Member

@jcoyne This PR is +1 for me, though. We could handle the scenario of supporting isMemberOfRelation as a separate PR.

@hectorcorrea
Copy link
Member

@jcoyne I didn't realize the Duraspace Wiki page does not open all the child pages by default. Search for "Ordering In Action" instead, open the "Ordering Creation Details..." link, and then you'll see the text that I mentioned under "Ordering - Create DirectContainer"

@jcoyne
Copy link
Member Author

jcoyne commented May 6, 2015

@hectorcorrea the latest version lets you set isMemberOfRelation

@jcoyne jcoyne force-pushed the attached_files branch 2 times, most recently from e2eacce to ccc019d Compare May 7, 2015 03:54
hectorcorrea
hectorcorrea reviewed May 7, 2015
View reviewed changes
lib/active_fedora/associations/builder/directly_contains.rb
def validate_options
super
if !options[:has_member_relation] && !options[:is_member_of_relation]
raise ArgumentError, "You must specify a predicate for #{name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{name} ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is name defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the superclass

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, nope. Good catch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, it is in the superclass: https://github.com/projecthydra/active_fedora/blob/master/lib/active_fedora/associations/builder/association.rb#L9

@jcoyne
Add direct containers
3facdba 
You can use the containers by calling:

class FooHistory < ActiveFedora::Base
   directly_contains :files, has_member_relation:
     ::RDF::URI.new("http://example.com/hasFiles"), class_name: 'Thing'
end

if you don't specify a class_name it will default to ActiveFedora::File
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 93.19% when pulling 3facdba on attached_files into 2c6f867 on master.

hectorcorrea
hectorcorrea reviewed May 7, 2015
View reviewed changes
spec/integration/direct_container_spec.rb
Object.send(:remove_const, :FooHistory)
end

let(:file) { o.files.build }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found it somewhat non-intuitive to use build rather than something line add or create but I guess since build is the ActiveRecord standard we should go with it. (sincerely, the department of pointless comments)

@hectorcorrea
Copy link
Member

@jcoyne The isMemberOfRelation implementation looks great. I documented[1] the questions that we had yesterday over IRC about how the containment is represented when using isMemberOfRelation vs hasMemberRelation in case somebody wonders about it later.

[1] https://gist.github.com/hectorcorrea/b7c80ce00914040c5ed7

@tpendragon
Copy link
Contributor

I'm 👍 at this point. Any lingering issues?

@jcoyne jcoyne mentioned this pull request May 7, 2015
Closed
tpendragon pushed a commit that referenced this pull request May 7, 2015
Merge pull request #788 from projecthydra/attached_files
36d4026 
Add direct containers
@tpendragon tpendragon merged commit 36d4026 into master May 7, 2015
@tpendragon tpendragon deleted the attached_files branch May 7, 2015 16:07
@jcoyne jcoyne mentioned this pull request May 7, 2015
Closed
@awead awead mentioned this pull request May 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jcoyne jcoyne

Labels
None yet
Projects
None yet
Milestone
May Hydra PCDM
Development

Successfully merging this pull request may close these issues.
6 participants
@jcoyne @awead @hectorcorrea @coveralls @tpendragon @no-reply